[GRPC] Upgrade to protobufs 1.0.0#20335
Conversation
📝 WalkthroughWalkthroughBumps opensearch-protobufs 0.24.0 → 1.0.0 and updates transport-grpc to align with protobuf breaking changes: rewrite fields become strings, aggregations/suggest removed from request protos, TermsLookup type changed, processor execution details added to responses, checksum files updated, and tests adjusted across the module. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as SearchResponseProducer
participant Utils as SearchResponseSectionsProtoUtils
participant PED as ProcessorExecutionDetail
participant OM as ObjectMapProtoUtils
participant Builder as SearchResponse.Builder
Note over Producer,Utils: Convert SearchResponse → protobuf SearchResponse
Producer->>Utils: toProto(searchResponse)
Utils->>PED: iterate processorResults (getName/getDuration/getStatus/getTag/getErrorMessage/getInput/getOutput)
PED-->>Utils: field values
Utils->>OM: convert input/output maps to ObjectMap proto
OM-->>Utils: ObjectMap proto
Utils->>Builder: add processor execution detail entries (name,duration,status,input,output,error,tags)
Builder-->>Producer: SearchResponse proto
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Signed-off-by: Karen X <karenxyr@gmail.com>
karenyrx
force-pushed
the
1.0.0protos
branch
from
7525a57 to
a010c06
Compare
Signed-off-by: Karen X <karenxyr@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20335 +/- ##
============================================
- Coverage 73.27% 73.20% -0.07%
+ Complexity 71739 71691 -48
============================================
Files 5785 5784 -1
Lines 328143 328141 -2
Branches 47270 47269 -1
============================================
- Hits 240445 240225 -220
- Misses 68397 68640 +243
+ Partials 19301 19276 -25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
karenyrx
force-pushed
the
1.0.0protos
branch
from
2971156 to
18ad919
Compare
karenyrx
requested review from
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/WildcardQueryBuilderProtoUtilsTests.java (1)
156-159: Update outdated comment.The comment claims that
fromProtohas"MULTI_TERM_QUERY_REWRITE_CONSTANT_SCORE"whilefromXContenthas"constant_score", but after the protobuf 1.0.0 upgrade, both now use the string"constant_score". This comment is misleading and should be updated or removed.🔎 Suggested fix
- // Note: The rewrite method is stored differently in the two builders - // fromXContent has "constant_score" while fromProto has "MULTI_TERM_QUERY_REWRITE_CONSTANT_SCORE" + // Note: Both fromXContent and fromProto now use the same string representation for rewrite assertEquals(fromXContent.boost(), fromProto.boost(), 0.001f);
🧹 Nitpick comments (6)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.java (1)
168-184: Consider testing unsigned overflow behavior.The test correctly validates UINT64_VALUE handling with
Long.MAX_VALUE, but doesn't cover the case where the protobuf uint64 value exceedsLong.MAX_VALUE(since Java'slongis signed). If the protobuf schema allows values in the range [2^63, 2^64-1], the current implementation would experience signed overflow when callinggetUint64Value().If this is intentional behavior (accepting overflow), consider adding a comment in the production code or a test documenting this edge case. If not, you may need to handle large unsigned values differently (e.g., using
BigIntegeror validating the range).modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsLookupProtoUtilsTests.java (1)
81-97: Consider if empty string test reflects meaningful behavior.This test passes empty strings for
index,id, andpath. While the proto accepts these values and the test verifies they pass through, the underlyingTermsLookupconstructor (per the relevant code snippets) throwsIllegalArgumentExceptionfor null values but accepts empty strings. Empty strings for these required fields likely don't represent valid use cases. Consider whether this test should verify that empty values are rejected or at least document that this tests proto passthrough rather than semantic validation.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/RegexpQueryBuilderProtoUtilsTests.java (1)
76-104: Tests correctly updated for string-based rewrite, consider simplifying test arrays.The test properly reflects the string-based rewrite API change. However, since
rewriteMethodsandexpectedRewriteMethodsare now identical (both are string arrays with the same values), you could simplify by using a single array.🔎 Suggested simplification
public void testFromProtoWithDifferentRewriteMethods() { // Test all possible rewrite methods String[] rewriteMethods = { "constant_score", "constant_score_boolean", "scoring_boolean", "top_terms_10", "top_terms_blended_freqs_10", "top_terms_boost_10" }; - String[] expectedRewriteMethods = { - "constant_score", - "constant_score_boolean", - "scoring_boolean", - "top_terms_10", - "top_terms_blended_freqs_10", - "top_terms_boost_10" }; - for (int i = 0; i < rewriteMethods.length; i++) { RegexpQuery proto = RegexpQuery.newBuilder() .setField("test_field") .setValue("test.*value") .setRewrite(rewriteMethods[i]) .build(); RegexpQueryBuilder builder = fromProto(proto); - assertEquals(expectedRewriteMethods[i], builder.rewrite()); + assertEquals(rewriteMethods[i], builder.rewrite()); } }modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtilsTests.java (1)
728-738: Test updated for protobufs 1.0.0 suggest field removal, but test name is now misleading.The update correctly reflects that the
suggestfield was removed fromSearchRequestBodyin protobufs 1.0.0. However, the test nametestParseProtoWithSuggestis now misleading since the test doesn't actually exercise suggest functionality—it just verifies that an empty proto parses without error.Consider renaming this test or removing it entirely, since it no longer tests suggest-specific behavior.
🔎 Suggested options
Option 1 - Remove the test (preferred if not adding value):
- public void testParseProtoWithSuggest() throws IOException { - // Suggester field was removed from SearchRequestBody in protobufs 1.0.0 - // Suggest functionality is now handled via SearchRequest URL parameters (suggest_field, suggest_text, etc.) - // This test is no longer applicable as the field doesn't exist - SearchRequestBody protoRequest = SearchRequestBody.newBuilder().build(); - - SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder(); - - // Should not throw exception as suggest field no longer exists - SearchSourceBuilderProtoUtils.parseProto(searchSourceBuilder, protoRequest, queryUtils); - }Option 2 - Rename to reflect actual test purpose:
- public void testParseProtoWithSuggest() throws IOException { + public void testParseProtoWithEmptyRequest() throws IOException { // Suggester field was removed from SearchRequestBody in protobufs 1.0.0 // Suggest functionality is now handled via SearchRequest URL parameters (suggest_field, suggest_text, etc.) - // This test is no longer applicable as the field doesn't exist SearchRequestBody protoRequest = SearchRequestBody.newBuilder().build(); SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder(); - // Should not throw exception as suggest field no longer exists + // Empty request should parse without error SearchSourceBuilderProtoUtils.parseProto(searchSourceBuilder, protoRequest, queryUtils); }modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
190-199: Consider removing unusedkeyparameter or documenting its retention.The
keyparameter is no longer used in the method body since the return type changed fromMap.Entry<String, StringOrStringArray>toStringArray. While the Javadoc mentions it's "kept for API compatibility," if this is an internal/protected method, you could simplify by removing the unused parameter. If external callers depend on this signature, the current approach is acceptable.🔎 Suggested simplification (if API compatibility not required)
- public static StringArray headerToProto(String key, List<String> values) throws IOException { + public static StringArray headerToProto(List<String> values) throws IOException { if (values != null && values.isEmpty() == false) { StringArray.Builder stringArrayBuilder = StringArray.newBuilder(); for (String val : values) { stringArrayBuilder.addStringArray(val); } return stringArrayBuilder.build(); } return null; }And update the call site at line 160:
- StringArray headerArray = headerToProto(entry.getKey(), entry.getValue()); + StringArray headerArray = headerToProto(entry.getValue());modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtilsTests.java (1)
110-164: Consider explicitly mocking null returns for clarity.While Mockito returns
nullby default for unmocked methods, explicitly settingwhen(mockResponse.getAggregations()).thenReturn(null)(and similar forgetSuggest()andgetProfileResults()) would make the test intent clearer and more maintainable.🔎 Proposed enhancement for test clarity
Add these mock setups after line 127 and line 155:
when(mockResponse.getClusters()).thenReturn(new SearchResponse.Clusters(0, 0, 0)); +when(mockResponse.getAggregations()).thenReturn(null); +when(mockResponse.getSuggest()).thenReturn(null); +when(mockResponse.getProfileResults()).thenReturn(null);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
CHANGELOG.mdgradle/libs.versions.tomlmodules/transport-grpc/licenses/protobufs-0.24.0.jar.sha1modules/transport-grpc/licenses/protobufs-1.0.0.jar.sha1modules/transport-grpc/spi/licenses/protobufs-0.24.0.jar.sha1modules/transport-grpc/spi/licenses/protobufs-1.0.0.jar.sha1modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/FuzzyQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/MatchBoolPrefixQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/MatchQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/MultiMatchQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/PrefixQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/RegexpQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsLookupProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/WildcardQueryBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/suggest/SuggestBuilderProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtils.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/FuzzyQueryBuilderProtoConverterTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/FuzzyQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/MatchBoolPrefixQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/MatchQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/MultiMatchQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/PrefixQueryBuilderProtoConverterTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/PrefixQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/RegexpQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsLookupProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/WildcardQueryBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/suggest/SuggestBuilderProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtilsTests.javaserver/src/main/java/org/opensearch/search/fetch/subphase/highlight/HighlightBuilder.javaserver/src/main/java/org/opensearch/search/pipeline/ProcessorExecutionDetail.javaserver/src/test/java/org/opensearch/search/pipeline/ProcessorExecutionDetailTests.java
💤 Files with no reviewable changes (6)
- modules/transport-grpc/spi/licenses/protobufs-0.24.0.jar.sha1
- modules/transport-grpc/licenses/protobufs-0.24.0.jar.sha1
- modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/suggest/SuggestBuilderProtoUtils.java
- modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtilsTests.java
- modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtils.java
- modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/suggest/SuggestBuilderProtoUtilsTests.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T18:40:08.452Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java
🧬 Code graph analysis (5)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtils.java (1)
HighlightBuilderProtoUtils(29-323)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsLookupProtoUtilsTests.java (2)
server/src/main/java/org/opensearch/indices/TermsLookup.java (1)
TermsLookup(60-269)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsLookupProtoUtils.java (1)
TermsLookupProtoUtils(18-49)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtils.java (1)
SearchResponseSectionsProtoUtils(23-105)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java (1)
SearchSourceBuilderProtoUtils(37-276)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
OpenSearchExceptionProtoUtils(47-251)
🔇 Additional comments (36)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/PrefixQueryBuilderProtoUtils.java (1)
52-54: LGTM: Direct string assignment aligns with protobuf schema change.The migration from enum-based rewrite handling to direct string assignment is correct and consistent with the protobufs 1.0.0 upgrade where
MultiTermQueryRewritechanged from enum to string type.modules/transport-grpc/spi/licenses/protobufs-1.0.0.jar.sha1 (1)
1-1: LGTM: Consistent checksum with main module.The checksum matches the protobufs-1.0.0.jar.sha1 file in the main transport-grpc module, which is correct since both modules use the same artifact.
CHANGELOG.md (1)
46-46: LGTM: Changelog entry is clear and well-formatted.The entry appropriately documents the protobufs version bump and mentions the transport-grpc module compatibility updates.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/MatchQueryBuilderProtoUtilsTests.java (1)
180-194: LGTM: Test correctly updated for string-based rewrite values.The test properly validates the fuzzy rewrite functionality using string literal
"constant_score"instead of enum values, aligning with the protobuf schema change where rewrite fields are now strings.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtils.java (1)
123-124: LGTM: UINT64_VALUE support correctly added.The implementation follows the existing pattern for numeric type handling. Note that Java's
longis signed, so protobuf uint64 values exceedingLong.MAX_VALUEwould experience signed overflow. This appears to be consistent with the handling of other numeric types in this utility.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtilsTests.java (2)
449-469: LGTM: Test renamed for clarity and enhanced with tag assertions.The test name
testFromProto_WithFieldTagsSchemaStyledbetter describes the specific schema being tested. The added assertions verify that the styled pre/post tags are correctly applied.
471-493: LGTM: Comprehensive test coverage for DEFAULT tags schema.The new test properly validates the
HIGHLIGHTER_TAGS_SCHEMA_DEFAULTcase, ensuring default pre/post tags are correctly applied to highlight fields.modules/transport-grpc/licenses/protobufs-1.0.0.jar.sha1 (1)
1-1: Manual verification required: The SHA-1 checksum495727dc62f515bd34e37dcdda976026b0aaf33acould not be verified against the officialorg.opensearch:protobufs:1.0.0artifact due to network access limitations. The checksum file format is correct and the version is consistent with the build configuration (gradle/libs.versions.toml), but the actual artifact on Maven Central should be verified to confirm the checksum is accurate.gradle/libs.versions.toml (1)
26-26: LGTM! Version bump is consistent with PR objectives.The upgrade from 0.24.0 to 1.0.0 aligns with the described protobuf schema changes throughout the PR.
server/src/main/java/org/opensearch/search/fetch/subphase/highlight/HighlightBuilder.java (1)
95-97: LGTM! Visibility change appropriately supports proto utilities.Making these constants public allows the proto conversion utilities to reference the default highlight tags. Since these are immutable arrays and the class is already part of the public API, this exposure is safe and aligns with the new HIGHLIGHTER_TAGS_SCHEMA_DEFAULT support.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/RegexpQueryBuilderProtoUtils.java (1)
51-53: LGTM! Rewrite field handling correctly updated for string type.The change from enum-to-string conversion to direct string assignment aligns with protobufs 1.0.0 where MultiTermQueryRewrite is now a string field. The removal of the UNSPECIFIED guard is appropriate since string fields can be checked with
hasRewrite().server/src/main/java/org/opensearch/search/pipeline/ProcessorExecutionDetail.java (1)
133-140: LGTM! Getters appropriately expose processor execution metadata.The new getters for
errorMessageandtagenable proto conversion utilities to access these fields for serialization. Since these fields are already serialized intoXContent(), exposing them via getters is consistent with the existing API.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtils.java (1)
315-318: LGTM! DEFAULT tags schema case correctly implemented.The new HIGHLIGHTER_TAGS_SCHEMA_DEFAULT case properly applies the default highlight tags by referencing the newly public
DEFAULT_PRE_TAGSandDEFAULT_POST_TAGSconstants. This aligns with the protobufs 1.0.0 enum addition.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/MultiMatchQueryBuilderProtoUtils.java (1)
136-138: LGTM! Fuzzy rewrite field handling correctly updated for string type.The change from enum-based conversion to direct string assignment aligns with the protobufs 1.0.0 schema where fuzzy_rewrite is now a string field. This is consistent with the pattern applied across other query builders in this PR.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/MatchBoolPrefixQueryBuilderProtoUtils.java (1)
93-95: LGTM! Fuzzy rewrite field handling correctly updated for string type.The change from enum-based conversion to direct string assignment aligns with the protobufs 1.0.0 schema where fuzzy_rewrite is now a string field. This completes the consistent pattern applied across all query builders in this PR.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/MatchBoolPrefixQueryBuilderProtoUtilsTests.java (1)
187-187: LGTM! Test correctly updated for string-based rewrite.The test now uses the string literal "constant_score" instead of the enum constant, aligning with the protobuf 1.0.0 schema change where rewrite values are represented as strings.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/PrefixQueryBuilderProtoUtilsTests.java (1)
36-36: LGTM! Test updated for string-based rewrite API.The change from enum to string literal is consistent with the protobuf schema upgrade.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/FuzzyQueryBuilderProtoConverterTests.java (1)
42-42: LGTM! Test correctly migrated to string-based rewrite.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/MatchQueryBuilderProtoUtils.java (1)
101-103: LGTM! Rewrite handling correctly migrated to string-based approach.The direct string assignment from the protobuf aligns with the schema change in protobufs 1.0.0. The
hasRewrite()check provides appropriate null safety.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/PrefixQueryBuilderProtoConverterTests.java (1)
37-37: LGTM! Test updated to match string-based rewrite API.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/FuzzyQueryBuilderProtoUtils.java (1)
74-76: LGTM! Rewrite handling properly updated to string-based extraction.The implementation correctly extracts the rewrite value as a string from the protobuf, consistent with the 1.0.0 schema changes.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/WildcardQueryBuilderProtoUtils.java (1)
58-60: LGTM! Rewrite extraction updated for string-based schema.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/WildcardQueryBuilderProtoUtilsTests.java (3)
54-54: LGTM! Test correctly uses string-based rewrite value.
89-117: LGTM! Test correctly validates string-based rewrite methods.The test appropriately reflects the protobuf 1.0.0 change where rewrite is now a string field.
144-144: LGTM! Test updated for string-based rewrite.server/src/test/java/org/opensearch/search/pipeline/ProcessorExecutionDetailTests.java (1)
188-234: LGTM! Good test coverage for new getter methods.The tests properly cover both positive cases (when values are present) and negative cases (when values are null) for
getErrorMessage()andgetTag()methods. The test structure is consistent with existing tests in the file.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtils.java (1)
43-71: Well-structured processor execution detail conversion.The implementation correctly handles null checks for all optional fields and follows the pattern of the existing code. Using
Locale.ROOTfor the status name conversion is appropriate for consistency across locales.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/MultiMatchQueryBuilderProtoUtilsTests.java (1)
431-474: Tests correctly updated for string-based fuzzyRewrite.The test updates properly reflect the API change where
fuzzyRewriteis now a string instead of an enum. The coverage for various rewrite string values (constant_score,constant_score_boolean,scoring_boolean,top_terms_N, etc.) is comprehensive.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsLookupProtoUtilsTests.java (1)
16-34: Tests correctly updated for TermsLookup proto type rename.The test updates properly reflect the
TermsLookupField→TermsLookuprename in protobufs 1.0.0. The comment on line 17 documenting this change is helpful for future maintainers.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/FuzzyQueryBuilderProtoUtilsTests.java (1)
37-65: Test correctly updated for string-based rewrite.The test properly reflects the API change where
rewriteis now a string instead of an enum, and correctly verifies that"constant_score"passes through to the builder.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
158-165: Header conversion correctly updated for StringArray type.The updated header handling properly uses
StringArrayinstead of the removedStringOrStringArraytype. The null check before putting the header entry is appropriate.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsLookupProtoUtils.java (1)
25-48: LGTM!The migration from
TermsLookupFieldtoTermsLookupproto type is correctly implemented. All field accesses have been properly updated, and the logic remains functionally equivalent.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtilsTests.java (2)
30-108: LGTM!Comprehensive test coverage for processor execution details with varying field combinations. Properly validates conversion of processor name, duration, input/output data, status (lowercased), error, and tag fields.
166-380: LGTM!Excellent test coverage for edge cases:
- Null field handling in processor details (input/output data, processor name, status, error, tag)
- UnsupportedOperationException validation for aggregations, suggest, profile results, and search extension builders
The tests properly verify both the presence and absence of optional fields using
has*()methods.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (2)
166-218: LGTM!Header conversion tests correctly updated to use
StringArrayproto type. Proper validation of:
- Single value conversion
- Multiple value conversion
- Null return for empty lists
- Null return for null values
466-532: LGTM!Good test coverage for additional edge cases:
- Metadata inclusion for exceptions with metadata (FailedNodeException)
- Header conversion to StringArray in ErrorCause
- Wrapped exception handling
- Custom metadata with OpenSearch prefix key
The tests properly validate the integration between header conversion and the overall exception proto transformation.
...n/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java
Show resolved
Hide resolved
.../opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java
Show resolved
Hide resolved
Signed-off-by: Karen X <karenxyr@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (1)
498-511: Consider testing the actual unwrap scenario or clarifying test intent.The test name and comment suggest it's testing the
toProtounwrap path whereExceptionsHelper.unwrapCausereturns a different exception, but it only callsgenerateThrowableProtodirectly. This doesn't exercise the unwrap logic intoProto.Consider either:
- Testing the actual unwrap scenario by calling
toProtowith an exception that triggers unwrapping, or- Renaming the test to reflect that it's testing
generateThrowableProtowith nested exceptions (though this is already covered bytestToProtoWithNestedOpenSearchException).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
🔇 Additional comments (9)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (9)
21-21: LGTM!The new imports support the migration to
StringArrayfor header representation and the new test case with custom metadata.Also applies to: 31-31
166-178: LGTM!The test correctly validates the migration from
StringOrStringArraytoStringArrayfor single-value headers.
180-194: LGTM!The test correctly verifies that multiple header values are preserved in order within the
StringArray.
196-218: LGTM!Both tests correctly verify that
headerToProtoreturnsnullfor empty or null value lists, preventing unnecessary proto objects.
434-448: LGTM!The test appropriately verifies that empty headers are filtered out during proto conversion, maintaining consistency with the
headerToProtobehavior.
450-464: LGTM!The test now correctly validates null header values (as opposed to empty lists), providing distinct coverage. The previous review concern has been addressed.
466-477: LGTM!The test appropriately verifies that exception metadata (such as
node_idfromFailedNodeException) is correctly included in the proto conversion.
479-496: LGTM!The test thoroughly validates header inclusion and correctly uses the
StringArrayAPI to verify both header presence and multi-value content.
513-532: LGTM!The test appropriately validates the handling of metadata with the
OPENSEARCH_PREFIX_KEYprefix using an anonymous subclass approach, ensuring this specific code path is covered.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (1)
498-511: Test name and comment don't match implementation.The test name
testToProtoWithWrappedExceptionand comment suggest testing exception unwrapping logic, but the implementation simply callsgenerateThrowableProtodirectly. This doesn't validate any unwrapping behavior mentioned in the comment.Consider either:
- Renaming to
testGenerateThrowableProtoWithNestedExceptionto match what it actually tests- Or restructuring to actually test the unwrapping code path in
toProto()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java
🧬 Code graph analysis (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
OpenSearchExceptionProtoUtils(47-251)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
🔇 Additional comments (5)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (5)
21-21: LGTM! Imports correctly updated for protobuf 1.0.0.The
StringArrayimport replaces the removedStringOrStringArraytype, andHashMapsupports the new metadata test.Also applies to: 31-31
166-218: LGTM! Tests correctly updated for new StringArray return type.The
headerToPrototests properly validate the API change:
- Single and multiple value cases correctly use
getStringArrayCount()andgetStringArray(i)- Empty and null input cases correctly expect
nullreturn
434-464: LGTM! Empty and null header handling validated correctly.Both tests properly verify that empty and null header values are filtered out during proto conversion, consistent with
headerToProtoreturningnullfor such inputs. The null header test was correctly addressed based on previous review feedback.
466-496: LGTM! Comprehensive integration tests for metadata and headers.These tests effectively validate end-to-end proto conversion:
- Metadata test confirms exception metadata flows through to
ErrorCause- Headers test confirms multi-value headers are correctly stored as
StringArrayin the proto
513-532: LGTM! Metadata prefix handling validated effectively.The anonymous inner class approach appropriately tests the code path that processes metadata entries with
OPENSEARCH_PREFIX_KEY, ensuring prefixed metadata flows through to the proto.
Signed-off-by: Karen X <karenxyr@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdgradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: Mend Security Check
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
|
❌ Gradle check result for 9050bdb: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
varunbharadwaj
left a comment
varunbharadwaj
left a comment
There was a problem hiding this comment.
Looks good to me overall.
|
❌ Gradle check result for 9050bdb: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
3 participants
Description
Fix compilation errors (removed/changed fields), and add new fields added in 0.24.0->1.0.0 protobufs upgrade
opensearch-project/opensearch-protobufs@0.24.0...1.0.0#diff-2b931e620077c456566a9d1378628816a249e38700b6ca55bbea3e8ec080f1d1
Major Schema Changes
Removed Fields
SearchRequestBody(moved to URL parameters or separate endpoint)Suggesterfield (moved to URL parameters)SearchResponseRenamed Types
Enum Value Additions
uint64_valuefieldVERSION_TYPE_FORCE(not supported in OpenSearch, handled asINTERNAL)FIELD_TYPE_SEMANTICHIGHLIGHTER_TAGS_SCHEMA_DEFAULTType Changes
stringtyperewriteandfuzzy_rewritefields in:WildcardQueryRegexpQueryPrefixQueryFuzzyQueryMatchQueryMatchBoolPrefixQueryMultiMatchQueryCommon Type Changes
StringArrayfor header values in error responsesRelated Issues
Part of #19569
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.